Repository navigation
fix(layout): place the next grid row below the tallest item - #2520
Open
MarkusAbtion wants to merge 1 commit into
Open
MarkusAbtion wants to merge 1 commit into
MarkusAbtion wants to merge 1 commit into
Conversation
A stale minHeight left over from previous data could exclude the row's real tallest item from the tallest check. The uneven-row branch then reset the min heights but returned the shorter item, so the next row started too high and overlapped the taller items, and nothing corrected it afterwards. Fixes Shopify#2519
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #2519
In a grid, layouts are kept per index across data changes, including
minHeight. When a new item at an index measures exactly its staleminHeight,processAndReturnTallestItemInRowskips it as a tallest candidate (layout.height > layout.minHeight), so a shorter item becomestallestItem. The uneven-row branch (maxHeight - tallestItem.height > 1) then resets the min heights, but it still returned that shorter item.recomputeLayoutsstarts the next row attallestItem.y + tallestItem.height, so the next row overlapped the taller items, and since nothing resizes afterwards, it stayed that way.The fix: in that branch, return the row's tallest measured item. Only the uneven-row path changes; rows where the check already found the tallest item behave as before.
Affected package:
@shopify/flash-list(GridLayoutManager).Reviewers’ hat-rack 🎩
yarn jest src/__tests__/GridLayoutManager.test.ts: the new test ("should place the next row below the tallest item after data changes") fails onmainwithExpected: 437, Received: 408and passes with this change.yarn test, 200 tests),yarn type-checkandyarn lintpass locally.minHeightis reset to 0 for the whole row interacts well with the repaint that follows. In my testing the next measurement pass keeps the row at the tallest item's height.Screenshots or videos (if needed)
Seen on web in a 3-column grid of cards whose titles wrap to one or two lines: after a search changed
data, cards with two-line titles were covered by the next row by one line height. With this change, or withclearLayoutCacheOnUpdate()called on data change as a workaround, the overlap is gone.